Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ENH: Explicit configuration API properties. #841

Merged
merged 4 commits into from
Feb 9, 2025
Merged

ENH: Explicit configuration API properties. #841

merged 4 commits into from
Feb 9, 2025

Conversation

mfendeksilverstripe
Copy link
Contributor

ENH: Explicit configuration API properties.

Description

  • Added explicit configuration API properties for existing settings
  • Minor code refactor for tests (DRY code cleanup)
  • No functional changes overall

Pull request checklist

  • The target branch is correct
  • All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
    • Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
  • The commit messages follow our commit message guidelines
  • The PR follows our contribution guidelines
  • Code changes follow our coding conventions
  • This change is covered with tests (or tests aren't necessary for this change)
  • Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
  • CI is green

@mfendeksilverstripe mfendeksilverstripe force-pushed the bugfix/configuration-api branch from f39b9c2 to 0352fe6 Compare April 8, 2024 04:21
@mfendeksilverstripe
Copy link
Contributor Author

Hey @GuySartorelli , need some guidance with this one as this is technically just a code refactor with no functional changes. I did add new configuration API properties but these are for existing settings.

@GuySartorelli
Copy link
Contributor

Yup no problem. What guidance are you after specifically?

@mfendeksilverstripe mfendeksilverstripe force-pushed the bugfix/configuration-api branch from 0352fe6 to 973526e Compare April 8, 2024 19:11
@mfendeksilverstripe
Copy link
Contributor Author

This is a non-functional code refactor change-set. Which base branch should be used @GuySartorelli ?

@GuySartorelli
Copy link
Contributor

GuySartorelli commented Apr 8, 2024 •
edited
Loading

Refactors which don't have API breaking changes should target the next minor release (so 7 in this case)

@GuySartorelli
Copy link
Contributor

@mfendeksilverstripe Sorry it's taken so long to look at this - can you please rebase it to resolve the merge conflict?

@mfendeksilverstripe
Copy link
Contributor Author

I've completed the rebase @GuySartorelli , please review.

GuySartorelli
GuySartorelli requested changes Feb 4, 2025
View reviewed changes
@mfendeksilverstripe
Copy link
Contributor Author

@GuySartorelli I've pushed up new changes, please review (PR feedback).

GuySartorelli
GuySartorelli requested changes Feb 9, 2025
View reviewed changes
Copy link
Contributor

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of small changes left. Sorry I didn't notice these last review.

@mfendeksilverstripe
Copy link
Contributor Author

Have added new changes as per your suggestion, please review @GuySartorelli

@GuySartorelli
Copy link
Contributor

GuySartorelli commented Feb 9, 2025 •
edited
Loading

CI failures are due to a problem with the way we set up our CI matrix and not a reflection of failures this PR creates.
I'll set up a separate CI run to check the tests all go green before merging.
See https://github.com/creative-commoners/silverstripe-fluent/actions/runs/13229729777

GuySartorelli
GuySartorelli approved these changes Feb 9, 2025
View reviewed changes
Copy link
Contributor

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, CI is green (see link above)

@GuySartorelli GuySartorelli merged commit f038d52 into 7 Feb 9, 2025
15 of 20 checks passed
@GuySartorelli GuySartorelli deleted the bugfix/configuration-api branch February 9, 2025 21:48
@GuySartorelli
Copy link
Contributor

GuySartorelli commented Feb 9, 2025 •
edited
Loading

PR merged.
Please remember that the colon after the commit message prefix isn't needed - and please use a fork for your PR next time. I think that might be at least part of why the CI run didn't work. Branches directly in this repo also get added to packagist, which we don't want for short-lived PR branches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuySartorelli GuySartorelli GuySartorelli approved these changes

Assignees
No one assigned
Labels
affects/77 change/minor effort/easy impact/low type/enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mfendeksilverstripe @GuySartorelli